peer: bound peer-controlled resource growth - #11090
Conversation
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟢 Low (3 files)
AnalysisThe substantive change is entirely in To override, add a |
yyforyongyu
added
backport-v0.20.x-branch
85a430b to
bbada64
Compare
|
|
||
| // pongReplyLimiter and pingFloodLimiter enforce the two-tier inbound | ||
| // ping policy for this connection. | ||
| pongReplyLimiter *rate.Limiter |
There was a problem hiding this comment.
just name it pongLimiter and pingLimiter?
| // outgoingMsg interface box, and list element, which we round up here. | ||
| // Charging this overhead makes cheap-message floods visible to the byte | ||
| // budget instead of treating shared Pong payloads as free. | ||
| queuedMsgOverhead = 128 |
There was a problem hiding this comment.
all these consts are flying around, i wonder if we could turn them into a struct or config
| } | ||
| popMsg(front) | ||
| case msg := <-p.outgoingQueue: | ||
| if msg.priority { |
There was a problem hiding this comment.
i wonder if we should stop modifying this loop? feels like a layer violation as the abstraction is broken? the queuing layer should not be aware of the ping/pong context?
Bound Pong replies with separate reply and flood token buckets while disconnecting peers that exhaust the flood budget. Add deterministic coverage for honest cadences, reply suppression, and flood teardown.
Collect the count, retained-memory, and fixed-overhead thresholds in one private policy value. Keep wire-specific cost estimation with that policy so the generic queue does not need Ping or Pong knowledge.
Calculate retained-memory charges when outgoing messages enter the peer queue boundary and carry the result as immutable metadata. Add a generic priority queue that accounts with those charges instead of inspecting wire message types.
Route queueHandler through the bounded queue abstraction and disconnect when either the count or retained-memory budget is exceeded. Use one nil-gated send case for empty and non-empty states, and keep servicing producers during teardown so disconnect cannot deadlock.
Cover count and retained-memory overflow, cost accounting, healthy queue draining, and sender service during teardown. The focused cases exercise the production queue boundary without starting unrelated peer loops.
Document the ping reply rate and outgoing queue bounds in both supported patch release notes so operators know that peer-controlled resource exhaustion is mitigated.
bbada64 to
1fef25c
Compare
1 participant
Change Description
Bound the resources a peer can consume through inbound pings and queued
outgoing messages.
Inbound ping handling now has two limits:
that reply budget are accepted without a Pong.
This flood check also covers pings whose requested Pong size would otherwise
be ignored.
The per-peer outgoing backlog is also capped at 10,000 messages and 16 MiB of
charged memory. Queue accounting covers fixed overhead and the high-risk
dynamic payloads without serializing messages on the hot path. An overflowing
peer is disconnected while the queue remains serviced until teardown, avoiding
deadlocks with synchronous senders.
Release notes are included for 0.21.3 and 0.20.4.
Open Question
The reply budget deliberately deviates from BOLT 1's requirement to answer
valid pings. A peer using those pings for liveness may close the connection when
its Pong is skipped, so the 1-10 ping/second band can relocate the disconnect
decision to the remote peer and make lnd appear unresponsive. Should the two
tiers remain, or should they be collapsed into the flood disconnect threshold?
This draft implements both tiers so reviewers can evaluate the concrete
behavior.
Steps to Test
The queue-accounting and teardown-drain regression tests were also mutation
tested by removing their respective implementation lines and confirming each
test failed.
Pull Request Checklist
Testing
Code Style and Documentation